[Role] Feature: Add az role deny-assignment create/delete commands#33109
Open
jruttle wants to merge 13 commits into
Open
[Role] Feature: Add az role deny-assignment create/delete commands#33109jruttle wants to merge 13 commits into
az role deny-assignment create/delete commands#33109jruttle wants to merge 13 commits into
Conversation
❌AzureCLI-FullTest
|
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| role deny-assignment | sub group role deny-assignment added |
Collaborator
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class az role deny-assignment CRUD support (focused on PP1 user-assigned deny assignments) to align with existing role assignment workflows, including command registration, parameters, help, and tests.
Changes:
- Register
az role deny-assignment list/show/create/deletecommands and add a table transformer for list output. - Implement deny-assignment list/show/create/delete custom handlers with PP1 validation.
- Add help/params, linter exclusions for long options, and introduce a new deny-assignment test file.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/role/custom.py |
Adds deny-assignment list/show/create/delete implementations and PP1 input validation. |
src/azure-cli/azure/cli/command_modules/role/commands.py |
Registers new role deny-assignment commands and list table transformer. |
src/azure-cli/azure/cli/command_modules/role/_params.py |
Defines CLI parameters for deny-assignment commands. |
src/azure-cli/azure/cli/command_modules/role/_help.py |
Adds help text and examples for the new/updated deny-assignment commands. |
src/azure-cli/azure/cli/command_modules/role/linter_exclusions.yml |
Excludes option-length lint rules for new long parameter names. |
src/azure-cli/azure/cli/command_modules/role/tests/latest/test_deny_assignment.py |
Adds scenario/live tests covering list/show and PP1 create/delete validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+627
to
+638
| 'deny_assignment_name': deny_assignment_name, | ||
| 'description': description or '', | ||
| 'permissions': [{ | ||
| 'actions': actions or [], | ||
| 'not_actions': not_actions or [], | ||
| 'data_actions': [], | ||
| 'not_data_actions': [] | ||
| }], | ||
| 'scope': scope, | ||
| 'principals': principals, | ||
| 'exclude_principals': exclude_principals, | ||
| 'is_system_protected': False |
There was a problem hiding this comment.
create_deny_assignment builds the request body as a plain dict using snake_case keys (e.g. deny_assignment_name, not_actions, exclude_principals). For mgmt SDK operations, dicts are typically serialized as-is, so the service will receive incorrect field names (it expects camelCase JSON, or a proper SDK model instance). Use the azure-mgmt-authorization model types via get_sdk(..., mod='models') (similar to RoleApiHelper.create_role_assignment) or ensure the payload keys match the service JSON contract exactly.
Suggested change
| 'deny_assignment_name': deny_assignment_name, | |
| 'description': description or '', | |
| 'permissions': [{ | |
| 'actions': actions or [], | |
| 'not_actions': not_actions or [], | |
| 'data_actions': [], | |
| 'not_data_actions': [] | |
| }], | |
| 'scope': scope, | |
| 'principals': principals, | |
| 'exclude_principals': exclude_principals, | |
| 'is_system_protected': False | |
| 'denyAssignmentName': deny_assignment_name, | |
| 'description': description or '', | |
| 'permissions': [{ | |
| 'actions': actions or [], | |
| 'notActions': not_actions or [], | |
| 'dataActions': [], | |
| 'notDataActions': [] | |
| }], | |
| 'scope': scope, | |
| 'principals': principals, | |
| 'excludePrincipals': exclude_principals, | |
| 'isSystemProtected': False |
Comment on lines
+641
to
+642
| return deny_client.create(scope=scope, deny_assignment_id=assignment_name, | ||
| parameters=deny_assignment_params) |
There was a problem hiding this comment.
This calls authorization_client.deny_assignments.create(...), but the repo currently pins azure-mgmt-authorization==5.0.0b1 (which does not include denyAssignments create/delete per the PR description). Without bumping the SDK dependency (or adding a fallback implementation / friendly error), this command will raise AttributeError at runtime.
Comment on lines
+645
to
+654
| def delete_deny_assignment(cmd, scope=None, deny_assignment_id=None, deny_assignment_name=None): | ||
| """Delete a user-assigned deny assignment.""" | ||
| authorization_client = _auth_client_factory(cmd.cli_ctx, scope) | ||
| deny_client = authorization_client.deny_assignments | ||
|
|
||
| if deny_assignment_id: | ||
| return deny_client.delete_by_id(deny_assignment_id) | ||
| if deny_assignment_name and scope: | ||
| return deny_client.delete(scope=scope, deny_assignment_id=deny_assignment_name) | ||
| raise CLIError('Please provide --id, or both --name and --scope.') |
There was a problem hiding this comment.
Same dependency issue as create: deny_client.delete(...)/delete_by_id(...) will fail at runtime unless the pinned azure-mgmt-authorization version includes these methods. Consider either updating the dependency in this PR or detecting missing methods and raising a clear CLIError instructing users to upgrade.
Comment on lines
+397
to
+398
| c.argument('deny_assignment_name', options_list=['--name', '-n'], | ||
| help='The display name of the deny assignment.') |
There was a problem hiding this comment.
deny_assignment_name is defined at the role deny-assignment group level, which makes --name/-n show up for subcommands like list even though list_deny_assignments doesn't accept that parameter. If a user supplies --name on list, the handler will receive an unexpected kwarg and fail. Recommend removing deny_assignment_name from the group context and defining --name only on show/create/delete where it is supported.
Suggested change
| c.argument('deny_assignment_name', options_list=['--name', '-n'], | |
| help='The display name of the deny assignment.') |
| c.argument('exclude_principal_ids', nargs='+', options_list=['--exclude-principal-ids'], | ||
| help='Space-separated list of principal object IDs to exclude from the deny. ' | ||
| 'At least one is required for user-assigned deny assignments.') | ||
| c.argument('exclude_principal_types', nargs='+', options_list=['--exclude-principal-types'], |
There was a problem hiding this comment.
--exclude-principal-types is documented as having accepted values, but the argument doesn't enforce them. To keep validation consistent with role assignment create --assignee-principal-type, use arg_type=get_enum_type([...]) (or an Enum) so invalid values are caught client-side with a clear error.
Suggested change
| c.argument('exclude_principal_types', nargs='+', options_list=['--exclude-principal-types'], | |
| c.argument('exclude_principal_types', nargs='+', options_list=['--exclude-principal-types'], | |
| arg_type=get_enum_type(['User', 'Group', 'ServicePrincipal']), |
Comment on lines
+15
to
+35
| class DenyAssignmentListTest(ScenarioTest): | ||
| """Tests for az role deny-assignment list — works on any subscription.""" | ||
|
|
||
| def test_deny_assignment_list(self): | ||
| """List deny assignments at the subscription scope.""" | ||
| result = self.cmd('role deny-assignment list').get_output_in_json() | ||
| # Result should be a list (may be empty if no deny assignments exist) | ||
| self.assertIsInstance(result, list) | ||
|
|
||
| def test_deny_assignment_list_with_scope(self): | ||
| """List deny assignments at a specific scope.""" | ||
| self.cmd('role deny-assignment list --scope /subscriptions/{sub}', | ||
| checks=[self.check('type(@)', 'array')]) | ||
|
|
||
| def test_deny_assignment_list_with_filter(self): | ||
| """List deny assignments with OData filter.""" | ||
| result = self.cmd( | ||
| 'role deny-assignment list --filter "atScope()"' | ||
| ).get_output_in_json() | ||
| self.assertIsInstance(result, list) | ||
|
|
There was a problem hiding this comment.
This file adds ScenarioTest cases for role deny-assignment list that will require a new VCR recording to pass in playback mode, but no corresponding recording YAML is added under tests/latest/recordings. Either add recordings for these scenario tests or convert them to mock-based tests (or LiveScenarioTest if they must be live-only) so CI doesn't fail.
Comment on lines
+56
to
+61
| self.kwargs.update({ | ||
| 'scope': '/subscriptions/{sub}', | ||
| 'name': 'CLI Test Deny Assignment', | ||
| 'action': 'Microsoft.Authorization/roleAssignments/write', | ||
| # Use a well-known object ID for exclusion (replace with a real SP in your test env) | ||
| 'exclude_id': self.create_guid() |
There was a problem hiding this comment.
exclude_id is set to self.create_guid(), which is a random GUID and is unlikely to correspond to a real principal in the tenant. If the denyAssignment API validates excluded principals, this live test will fail reliably. Prefer creating a real principal in the test setup (or using the signed-in user/service principal object id) and passing its object ID here.
Suggested change
| self.kwargs.update({ | |
| 'scope': '/subscriptions/{sub}', | |
| 'name': 'CLI Test Deny Assignment', | |
| 'action': 'Microsoft.Authorization/roleAssignments/write', | |
| # Use a well-known object ID for exclusion (replace with a real SP in your test env) | |
| 'exclude_id': self.create_guid() | |
| signed_in_user = self.cmd('ad signed-in-user show').get_output_in_json() | |
| exclude_id = signed_in_user['id'] | |
| self.kwargs.update({ | |
| 'scope': '/subscriptions/{sub}', | |
| 'name': 'CLI Test Deny Assignment', | |
| 'action': 'Microsoft.Authorization/roleAssignments/write', | |
| 'exclude_id': exclude_id |
| # These tests require a subscription with the UserAssignedDenyAssignment feature flag enabled. | ||
|
|
||
| import unittest | ||
| from unittest import mock |
There was a problem hiding this comment.
Unused import: from unittest import mock is not used in this test file. Please remove it to keep linting clean.
Suggested change
| from unittest import mock |
Update create command to support two modes: - Everyone mode (default): denies all principals, requires exclude-principal-ids - Per-principal mode: denies a specific User or ServicePrincipal via --principal-id/--principal-type API changes from DA PR msazure/One#15293894: - 3P UADA can now target specific User and ServicePrincipal principals - Group type principals are explicitly disallowed - Single-principal-per-UADA constraint enforced Changes: - custom.py: Add principal_id/principal_type params, dual-mode logic, Group rejection - _params.py: Add --principal-id and --principal-type (enum) arguments - _help.py: Update long-summary and examples for both modes - tests: Add per-principal CRUD, Group rejection, missing-param validation tests Co-authored-by: Copilot <[email protected]>
jruttle
force-pushed
the
jruttle/add-deny-assignment-commands
branch
from
ae59ca3 to
01b4d90
Compare
Now that azure-mgmt-authorization 5.0.0b2 has been published to PyPI (2026-05-07), this picks up the new DenyAssignment management plane operations (BeginCreateOrUpdate / BeginDelete) added in PR #46223. This unblocks the deny-assignment CLI commands in this PR from running against the new SDK surface.
jruttle
requested review from
DanielMicrosoft,
ReaNAiveD and
bebound
as code owners
Member
Author
|
Bumped This picks up the new DenyAssignment management-plane operations ( This was the last upstream blocker on this PR — the CLI commands here now have a real published SDK surface to call against. CC for review awareness: prior reviewers / CLI mgmt module owners. |
Follow-up to commit 20fe3a7 which only bumped setup.py. The Linux/Darwin/Windows requirements lock files also pinned 5.0.0b1 and caused azdev-linter / azdev-style CI to fail with: ERROR: Cannot install azure-cli==2.85.0 and azure-mgmt-authorization==5.0.0b1 because these package versions have conflicting dependencies. This commit aligns all three platform lock files with setup.py at 5.0.0b2.
Member
Author
|
Follow-up to commit …showed the platform lock files ( CI should now re-run cleanly on the new HEAD. |
Member
Author
|
/azp run |
|
Commenter does not have sufficient privileges for PR 33109 in repo Azure/azure-cli |
Member
Author
|
Hi @yonzhan @isra-fel — could one of you (or anyone with write access) re-trigger the Azure DevOps validation pipelines on this PR? The two required checks Either of these comments should kick them off:
Background context: the SDK dependency bump unblocks this PR — Thanks! |
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
…ract changes The new TypeSpec-generated 5.0.0b2 SDK introduced breaking changes that block the Full Test pipeline. Changes: * role/custom.py _resolve_role_id: pass `filter` as a keyword argument to RoleDefinitionsOperations.list (its signature is now `list(scope, *, filter=None)`). This single line was the root cause for the wide blast radius across vm/iot/aro/ams/acr/resource modules and the role module's own RoleAssignmentScenarioTest cases - all of them resolve roles by name and therefore go through this function. * role/custom.py update_role_assignment: replace `RoleAssignment.from_dict(d)` with `RoleAssignment(d)`. The new Model base class accepts a JSON mapping directly and no longer exposes from_dict. * role/custom.py create_deny_assignment: switch from passing a snake_case dict to constructing SDK model objects (DenyAssignment, DenyAssignmentProperties, DenyAssignmentPermission, DenyAssignmentPrincipal). The TypeSpec serializer would have written the raw snake_case keys to the wire instead of camelCase. Also rename `deny_client.create(...)` to `create_or_update(...)` (the method was renamed in the new SDK). * role/custom.py delete_deny_assignment: the new SDK does not expose delete_by_id for deny assignments, so parse the resource ID via a new _parse_deny_assignment_id helper (case-insensitive regex that handles subscription, resource-group, and management-group scopes) and call delete(scope, deny_assignment_id). * role/custom.py show_deny_assignment / delete_deny_assignment / create_deny_assignment: move argument validation ahead of the _auth_client_factory(...) call so validation-only error paths don't require an authenticated session. * tests/latest/test_deny_assignment.py: convert DenyAssignmentListTest to LiveScenarioTest (these hit the live API and have no recorded cassettes in the repo), and replace DenyAssignmentShowTest with a unittest-based DenyAssignmentShowValidationTest that calls show_deny_assignment() directly and asserts the CLIError - both changes remove the dependency on a logged-in session in playback CI. Co-authored-by: Copilot <[email protected]>
jruttle
changed the title
az role deny-assignment create/delete commands
Member
Author
|
Pushed fix commit
Also updated the PR title to /azp run |
Extract _build_deny_assignment_model helper to keep the local count under the project's max-locals=25 threshold (was 26/25 in azdev-style after the SDK-model refactor). Co-authored-by: Copilot <[email protected]>
Member
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Pipeline build 314889 (re-triggered 11 May) still fails because the 8 May patch
did not address every breaking change in the new TypeSpec-generated SDK. This
commit fixes the four remaining classes of issue.
* role/custom.py update_role_assignment: `RoleAssignment(some_dict)` silently
produces a model with `properties=None` (the new model wraps domain
attributes under a nested `properties` field, with `__flattened_items`
exposing them via descriptors only when `properties` is set). The old code
then read `assignment.scope` -> None and passed scope=None to the SDK URL
serializer, which raised `ValueError('No value for given attribute')`.
Read scope/name/principalType/etc. directly from the user-supplied flat
camelCase dict and send a `{"properties": {...}}` JSON body to .create()
via its JSON overload - simpler and avoids the new model's flatten/unflatten
gymnastics. Caused test_role_assignment_create_update.
* role/custom.py list_role_assignments + list_deny_assignments +
show_deny_assignment: `knack.util.todict` walks `__dict__` and
therefore returns `{}` for the new MutableMapping-based models, so the
subsequent `ra['roleDefinitionId']` / `ra['principalId']` lookups
raised `KeyError`. Added explicit _role_assignment_to_dict and
_deny_assignment_to_dict adapters that project the model back to the
legacy flat camelCase shape (with enum -> str coercion) and routed all
list/show paths through them. Caused
test_create_for_rbac_password_with_assignment.
* vm/_validators.py, ams/operations/sp.py, containerapp/_utils.py,
acs/_roleassignments.py: `RoleDefinitionsOperations.list` is now
`list(scope, *, filter=None)`, so each remaining caller that passed
filter positionally raised
`TypeError: list() takes 2 positional arguments but 3 were given`.
This is the same bug fixed in 2d2c80a for role/custom.py::_resolve_role_id;
these four call sites were missed. Caused test_vm_msi, test_vm_explicit_msi,
test_vmss_msi, test_vmss_explicit_msi, test_ams_sp_create_reset, and the
ACS / Container Apps role-resolution paths.
Co-authored-by: Copilot <[email protected]>
jruttle
requested review from
FumingZhang,
Greedygre,
Juliehzl,
NoriZC,
howang-ms and
yanzhudd
as code owners
While running the previously-failing tests locally to verify 005aa58, test_identity_hub continued to fail because azure-cli-core itself has its own resolve_role_id helper at src/azure-cli-core/azure/cli/core/commands/arm.py that also passed filter positionally to RoleDefinitionsOperations.list. This is the shared utility many modules call via core.commands.arm.create_role_assignment. Same one-line fix as the four module-level callers patched in 005aa58: pass filter as a keyword argument so it works with the new TypeSpec-generated SDK signature `list(scope, *, filter=None)`. Local verification (azure-mgmt-authorization 5.0.0b2 installed): * test_role_assignment_create_update PASSED (was: ValueError) * test_role_assignment_scenario PASSED * test_role_assignment_create_using_principal_type PASSED * test_role_assignment_handle_conflicted_assignments PASSED * test_create_for_rbac_password_with_assignment PASSED (was: KeyError) * test_vm_msi PASSED (was: TypeError) * test_vm_explicit_msi PASSED * test_vmss_msi PASSED * test_vmss_explicit_msi PASSED * test_identity_hub PASSED (was: TypeError - this commit's fix) * Full role test sweep: 17 passed, 2 LiveOnly skipped * Role unit tests: 14 passed test_ams_sp_create_reset still fails with VCR cassette mismatch (the cassette was recorded against azure-mgmt-authorization 4.x but dev branch bumped to 5.0.0b1 in Azure#31859 without re-recording AMS cassettes; multiple identical requests now exhaust the single recorded response). This is pre-existing on dev and outside the scope of this PR. Co-authored-by: Copilot <[email protected]>
The azdev-style GitHub Actions check (run id 25725694563) failed flake8 on the new `_coerce` function added in 005aa58 because it followed the section comment block with only one blank line. Adding the second blank line resolves the two reported E302 errors at line 46. Verified locally with `azdev style --pep8` -> Flake8: PASSED. Co-authored-by: Copilot <[email protected]>
Member
Author
|
Hi @yonzhan @isra-fel — could one of you
I ran each of the previously-failing tests locally in playback against
One pre-existing test, Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DescriptionAdds
az role deny-assignment createandaz role deny-assignment deletecommands for managing user-assigned deny assignments, matching the existingaz role assignmentpattern.This implements the PUT/DELETE operations added in theMicrosoft.Authorization/denyAssignmentsAPI (2024-07-01-preview), as specified in TypeSpec PR #41617.### Two Assignment ModesThecreatecommand supports two modes for targeting principals:Everyone mode (default): Denies all principals at the scope. At least one excluded principal is required via--exclude-principal-ids:az role deny-assignment create --name "DenyAll" --scope /subscriptions/{sub} \ --actions "Microsoft.Compute/virtualMachines/delete" \ --exclude-principal-ids {your-object-id}Per-principal mode: Denies a specific User or ServicePrincipal via--principal-idand--principal-type:az role deny-assignment create --name "DenyUser" --scope /subscriptions/{sub} \ --actions "Microsoft.Compute/virtualMachines/delete" \ --principal-id {user-object-id} --principal-type User### Service ConstraintsUser-assigned deny assignments have specific restrictions enforced by the service:- Group principals are not permitted — only User or ServicePrincipal- No DataActions — only Actions/NotActions are supported- No Read actions — actions like*/readare not permitted- No DoNotApplyToChildScopes — this property is not supported- Single principal per UADA — one principal per deny assignment (enforced by backend)### Commands-az role deny-assignment list— List deny assignments (existing, enhanced)-az role deny-assignment show— Show a deny assignment (existing, enhanced)-az role deny-assignment create— Create a user-assigned deny assignment-az role deny-assignment delete— Delete a user-assigned deny assignment### Files Changed-commands.py— Command registration forrole deny-assignmentgroup-custom.py— Business logic with dual-mode principal handling and validation-_params.py— Parameter definitions including--principal-idand--principal-type-_help.py— Help text with examples for both Everyone and per-principal modes-linter_exclusions.yml— Exclusions for long parameter names-tests/latest/test_deny_assignment.py— Unit tests (list, show, CRUD, per-principal, Group rejection, param validation)### Dependency> Note: Thecreateanddeleteoperations depend on the Python SDK PR azure-sdk-for-python#46223 being merged first. This PR pinsazure-mgmt-authorizationto 5.0.0b2 (released 7 May 2026 on PyPI), which includes thecreate_or_updateanddeletemethods onDenyAssignmentsOperations.### TestingTests are included intest_deny_assignment.py. Full end-to-end testing requires:1. A subscription with theMicrosoft.Authorization/SubscriptionAllowedToOperateUserAssignedDenyAssignmentfeature flag registered2. The updated Python SDK with create/delete support### Related- TypeSpec PR: azure-rest-api-specs#41617 (merged)- Python SDK auto-gen PR: azure-sdk-for-python#46223 (awaiting review)- Go SDK auto-gen PR: azure-sdk-for-go#26544 (awaiting review)- Java SDK auto-gen PR: azure-sdk-for-java#48751 (awaiting review)- JS SDK auto-gen PR: azure-sdk-for-js#38079 (awaiting review)- PowerShell PR: azure-powershell#29340 (merged ✅)